Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2] feat(twap): generate all part orders #2759

Merged
merged 11 commits into from
Jul 5, 2023
Merged

[2] feat(twap): generate all part orders #2759

merged 11 commits into from
Jul 5, 2023

Conversation

shoom3301
Copy link
Collaborator

@shoom3301 shoom3301 commented Jun 30, 2023

Summary

Fixes #2640

Implemented generating of all TWAP order parts using the algorithm from #2640

To Test

It should be tested in the last PR

@vercel
Copy link

vercel bot commented Jun 30, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
swap-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback

🌃 Cosmos ↗︎

@shoom3301 shoom3301 requested a review from a team June 30, 2023 13:57
@shoom3301 shoom3301 self-assigned this Jun 30, 2023
@shoom3301 shoom3301 added the TWAP label Jun 30, 2023
@shoom3301 shoom3301 changed the title feat(twap): generate all part orders [2] feat(twap): generate all part orders Jun 30, 2023
)
const orders = Object.values(get(twapPartOrdersAtom))

return orders.flat().filter((order) => order.safeAddress === accountLowerCase && order.chainId === chainId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is order.safeAddress always lowercase?

const twapOrders = Object.values(twapOrdersList)

const ordersParts$ = twapOrders.map((twapOrder) => {
return generateTwapOrderParts(twapOrder, account.toLowerCase(), chainId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't you lowercaase inside the mehtod instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of performance, it's better to call toLowerCase() once. I would like to move the result of account.toLowerCase() in const outside of map(). Wdyt?

.map((_, index) => createPartOrderFromParent(twapOrder, index))
.filter(isTruthy)

const ids = await Promise.all(parts.map((part) => computeOrderUid(chainId, safeAddress, part as Order)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need to cast the part into an order?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately yes. Because computeOrderUid is based on @cowprotocol/contracts but, in the rest of CowSwap we mostly use types from @cowprotocol/cow-sdk.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately yes. Because computeOrderUid is based on @cowprotocol/contracts but, in the rest of CowSwap we mostly use types from @cowprotocol/cow-sdk.

@@ -9,11 +9,13 @@ const AUTH_THRESHOLD = ms`1m`
export function getTwapOrderStatus(
order: TWAPOrderStruct,
isExecuted: boolean,
executionDate: Date,
executionDateStr: string | null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not passing the Date or null directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks!
Fixed

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great changes!

@shoom3301 shoom3301 changed the base branch from fix/twap-handler-banner to develop July 4, 2023 10:47
@shoom3301 shoom3301 merged commit 7901e1a into develop Jul 5, 2023
20 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2023
@alfetopito alfetopito deleted the fix/2640-s-1 branch July 5, 2023 09:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Derive TWAP order filled %
3 participants